Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[stable6] Backport of stable 6 for Nextcloud 26 #3431

Merged
merged 27 commits into from
Apr 30, 2024
Merged

[stable6] Backport of stable 6 for Nextcloud 26 #3431

merged 27 commits into from
Apr 30, 2024

Conversation

come-nc
Copy link
Contributor

@come-nc come-nc commented Apr 15, 2024 •
edited by artonge
Loading

@come-nc come-nc self-assigned this Apr 15, 2024
@come-nc come-nc changed the base branch from master to stable-6 April 15, 2024 14:05
@dartcafe
Copy link
Collaborator

nice. Now I remember the incompatibility. 😁

@dartcafe
Copy link
Collaborator

I updated the branch protection for stable-6

@come-nc
Copy link
Contributor Author

come-nc commented Apr 16, 2024

nice. Now I remember the incompatibility. 😁

Do you think I missed anything? Do you know how complete are the unit tests?
I tested the UI a bit and it seems to work, but there is always the possibility of an incompatible signature in a corner-case code path.
Psalm seems happy so that’s good.

@dartcafe
Copy link
Collaborator

As soon as you are ready, add me as a reviewer. I will check then. Code wise it looks good.

@dartcafe
Copy link
Collaborator

I installed NC26 and will test some recent edge cases. But it may make sense to add the performance issues to the branch, so the stable-5 can die.

@come-nc come-nc marked this pull request as ready for review April 23, 2024 07:56
@come-nc come-nc requested a review from dartcafe April 23, 2024 07:56
artonge
artonge previously approved these changes Apr 24, 2024
View reviewed changes
.github/workflows/static-analysis.yml Show resolved Hide resolved
lib/Controller/VoteController.php Outdated Show resolved Hide resolved
lib/Controller/VoteController.php Outdated Show resolved Hide resolved
@come-nc @artonge
fix: Do not try to get participant from DB for null poll id
ea512e2 
Saves a few DB requests

Signed-off-by: Côme Chilliet <[email protected]>
@come-nc @artonge
fix: Do not get polls from DB twice
e4b6bd6 
Avoid a DB request for each poll, instead use the data we just got from
 the full list.

Signed-off-by: Côme Chilliet <[email protected]>
@come-nc @artonge
fix: Cache if no share is found for current poll in Acl
8bf95e8 
Not pretty but avoids a few DB calls. Ideally we should not do these
 requests at all, I do not really understand this share part in Acl.

Signed-off-by: Côme Chilliet <[email protected]>
artonge
artonge previously approved these changes Apr 24, 2024
View reviewed changes
@ArtificialOwl
Merge pull request #3445 from nextcloud/fix/stable-6-for-26-perf-impr…
3097e50 
…ovements

Perf improvements for stable-6 for 26 branch
@come-nc @artonge
fix: Cache if no share is found for current poll in Acl
65a23bc 
Not pretty but avoids a few DB calls. Ideally we should not do these
 requests at all, I do not really understand this share part in Acl.

Signed-off-by: Côme Chilliet <[email protected]>
artonge and others added 3 commits April 24, 2024 14:21
@artonge
Fix unit tests
faa7ecf 
Signed-off-by: Louis Chemineau <[email protected]>
@artonge
Fix cs:check
ec3d982 
Signed-off-by: Louis Chemineau <[email protected]>
@susnux @artonge
fix: Add visual loading feedback for polls list
78beb2c 
Signed-off-by: Ferdinand Thiessen <[email protected]>
@artonge artonge changed the title Backport of stable 6 for Nextcloud 26 [stable6] Backport of stable 6 for Nextcloud 26 Apr 24, 2024
artonge and others added 6 commits April 24, 2024 14:48
@artonge
Merge pull request #3446 from nextcloud/fix/stable-6-for-26-perf-back…
8d347b7 
…ports

[stable6] Backports of perf improvements for stable 6 on 26
@artonge
Merge pull request #3448 from nextcloud/fix/stable-6-backports-3444
62f054e 
[stable6] fix: Add visual loading feedback for polls list
@ArtificialOwl
reverse getToken()
3f1a90b 
Signed-off-by: Maxence Lange <[email protected]>
@ArtificialOwl
Merge pull request #3449 from nextcloud/revert-1eba
cbf617a 
reverse getToken()
@artonge
Default to "My polls" view
ad78cc5 
Signed-off-by: Louis Chemineau <[email protected]>
@artonge
Merge pull request #3451 from nextcloud/artonge/backport/stable6/3450
2d4b613 
[stable6] Default to "My polls" view
@dartcafe
Copy link
Collaborator

dartcafe commented Apr 26, 2024 •
edited
Loading

I did some smoke testing and did not find any flaws so far. So OK from me!

Do you know how complete are the unit tests?

The whole testing is very poor and would need some significant coverage increase. See #3147 😉

dartcafe
dartcafe previously approved these changes Apr 26, 2024
View reviewed changes
@come-nc
Copy link
Contributor Author

come-nc commented Apr 29, 2024 •
edited
Loading

@dartcafe dartcafe added this to the 6.2 milestone Apr 30, 2024
@dartcafe
Copy link
Collaborator

dartcafe commented Apr 30, 2024 •
edited
Loading

@dartcafe
Copy link
Collaborator

dartcafe commented Apr 30, 2024 •
edited
Loading

@come-nc @ChristophWurst What's about the open PRs? Aren't they superseeded and are to be closed?

@come-nc
Copy link
Contributor Author

come-nc commented Apr 30, 2024

  • Master is fine as it is, as well as the stable-6, or what do you have to do there?

You’re right master makes no sense, stable-6 is not compatible with Nextcloud master.
We could test with stable28 along with stable26 but I think testing the oldest supported version is enough for a stable version like this.

@come-nc
Copy link
Contributor Author

come-nc commented Apr 30, 2024

  • Please merge, whenever you are ready to (also to stable-6 ??)

I’m ready to merge, I just need your approval.

@dartcafe
Copy link
Collaborator

dartcafe commented Apr 30, 2024 •
edited
Loading

Approval already given. Ah. Needed approval again. Done.
What do you think about merging #3467 first to this branch?
It will help to reduce the poll loading time by half.

@dartcafe dartcafe self-requested a review April 30, 2024 15:33
@come-nc come-nc merged commit bce814e into stable-6 Apr 30, 2024
27 checks passed
@delete-merged-branch delete-merged-branch bot deleted the stable-6-for-26 branch April 30, 2024 15:46
@come-nc
Copy link
Contributor Author

come-nc commented Apr 30, 2024

What do you think about merging #3467 first to this branch?
It will help to reduce the poll loading time by half.

I think it’s cleaner to merge to stable-6.
This branch was meant as a temporary effort to test if supporting 26 was possible, now that it’s merged we can merge other things to stable-6 to keep improvements going.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dartcafe dartcafe dartcafe approved these changes

@artonge artonge artonge left review comments

Assignees

@come-nc come-nc

Labels
4. to release Performance
Projects
None yet
Milestone
6.3
Development

Successfully merging this pull request may close these issues.
5 participants
@come-nc @dartcafe @artonge @ArtificialOwl @susnux